Skip to content

fix: (test) changes to v20 support#2806

Closed
comatory wants to merge 8 commits intomainfrom
ondrej/eng-3812-implement-composedirective-directive
Closed

fix: (test) changes to v20 support#2806
comatory wants to merge 8 commits intomainfrom
ondrej/eng-3812-implement-composedirective-directive

Conversation

@comatory
Copy link
Copy Markdown
Contributor

@comatory comatory commented Apr 29, 2026

Test changes related to #2805

Summary by CodeRabbit

  • New Features

    • Added comprehensive validation and error reporting for directive composition, argument validation, and custom directive definitions.
  • Chores

    • Updated supported Node.js versions to 22.x and 24.x.
    • Upgraded TypeScript to 5.6.2.

Checklist

Open Source AI Manifesto

This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR refactors directive representation from "persisted directives" to "federated directives" across the composition system, introduces new directive-definition-data infrastructure with typed parameters and utilities, creates a BatchNormalizer class for batch normalization, establishes centralized execution result types, reorganizes error and validation APIs, and updates federation/normalization workflows accordingly.

Changes

Cohort / File(s) Summary
CI and Dependencies
.github/actions/node/action.yaml, .github/workflows/cli-ci.yaml, composition/package.json, pnpm-workspace.yaml
Updated Node.js test matrix versions (22.x, 24.x), added @types/node devDependency, bumped TypeScript from 5.5.2 to 5.6.2, and changed YAML quoting style in action metadata.
Core Execution Result Types
composition/src/types/results.ts, composition/src/types/types.ts
Added execution result interfaces (ExecutionMultiFailure, ExecutionSingleFailure, ExecutionSuccess) and union types, plus new Directive and DirectiveLocation type aliases.
Directive Definition Data Infrastructure
composition/src/directive-definition-data/directive-definition-data.ts, composition/src/directive-definition-data/types/params.ts, composition/src/directive-definition-data/types/types.ts, composition/src/directive-definition-data/utils.ts
New module exporting 33 directive definition data constants, parameter types (UpsertFederatedDirectiveDataParams, DirectiveDefinitionDataParams, etc.), type definitions (DirectiveArgumentData, DirectiveDefinitionData), and factory functions (newDirectiveArgumentData, newDirectiveDefinitionData).
Error and Validation Types
composition/src/errors/errors.ts, composition/src/errors/types/params.ts, composition/src/errors/types/types.ts
Expanded error API with 30+ new error constructors for directive composition/validation, replaced path with coords parameter naming, removed legacy exports, added new result-based error validation functions, and introduced InvalidRootTypeFieldEventsDirectiveData type.
Schema Building Refactor
composition/src/schema-building/ast.ts, composition/src/schema-building/params.ts, composition/src/schema-building/types/params.ts, composition/src/schema-building/types/results.ts, composition/src/schema-building/types/types.ts
Removed mutable directive/object extension helpers, added extractDirectiveLocations, refactored from "persisted" to "federated" directives, introduced new result types for schema-building operations, and created parameter type definitions module.
Schema Building Utilities
composition/src/schema-building/utils.ts
Comprehensive refactor replacing persisted-directive logic with federated-directive helpers, updated default-value comparison to return results, added getRouterSchemaDirectiveNodes, sanitizeDefaultValue, and router schema node builders (routerSchemaFieldNodeFromData, etc.).
Federation Core
composition/src/federation/federation.ts, composition/src/federation/types/params.ts, composition/src/federation/types/types.ts
Updated type import paths, refactored federation result types from type aliases to interfaces extending ExecutionSuccess/ExecutionMultiFailure, tightened map key typing to use domain types, and renamed persistedDirectivesData to federatedDirectivesData.
Normalization Refactor
composition/src/normalization/normalization.ts, composition/src/normalization/types.ts
Removed batchNormalize API, refactored result types to use execution base types, added federated/executable directive data fields, replaced string-keyed maps with typed keys, and removed persisted-directive fields.
Subgraph Types and Utils
composition/src/subgraph/types.ts, composition/src/subgraph/types/params.ts, composition/src/subgraph/utils.ts
Renamed conditionalFieldDataByCoordinates to conditionalFieldDataByCoords, changed entityInterfaces to entityInterfaceSubgraphDataByTypeName, added federatedDirectiveDataByName, removed persistedDirectiveDefinitionDataByDirectiveName, and added internalSubgraphFromNormalization converter.
Utility Enhancements
composition/src/utils/params.ts, composition/src/utils/string-constants.ts, composition/src/utils/utils.ts
Added MergeSetValueMapParams type, introduced LITERAL_AT constant, tightened Set<DirectiveName> and Set<FieldName> typing, renamed NON_REPEATABLE_PERSISTED_DIRECTIVES to NON_REPEATABLE_FEDERATED_DIRECTIVES, and added mergeSetValueMap helper.
Public API Exports
composition/src/index.ts
Reorganized exports with new directive-definition-data, error types, federation types, schema-building types/results modules; removed/replaced old barrel exports with granular type module paths.
V1 Federation Factory
composition/src/v1/federation/federation-factory.ts
Major refactor replacing persisted-directive aggregation with federated-directive handling, updated router schema construction with new result-returning helpers, switched to BatchNormalizer class, and updated internal subgraph map naming.
V1 Federation Support
composition/src/v1/federation/types/params.ts, composition/src/v1/federation/utils.ts, composition/src/v1/federation/walkers.ts
Updated type imports to new module paths, renamed ExtractPersistedDirectivesParams to ExtractFederatedDirectivesParams, added UpsertDirectiveArgumentDataParams, replaced persisted-directive logic with federated directives in walkers.
V1 Normalization Batch Processing
composition/src/v1/normalization/batch-normalization/batch-normalizer.ts, composition/src/v1/normalization/batch-normalization/types/params.ts
New BatchNormalizer class handling batch federation across subgraphs with override/entity/directive data merging, import deduplication, and comprehensive error/warning aggregation; added HandleOverridesParams type.
V1 Normalization Core Refactor
composition/src/v1/normalization/directive-definition-data.ts, composition/src/v1/normalization/normalization-factory.ts, composition/src/v1/normalization/types/params.ts, composition/src/v1/normalization/types/results.ts, composition/src/v1/normalization/types/types.ts, composition/src/v1/normalization/utils.ts, composition/src/v1/normalization/walkers.ts
Deleted old directive-definition-data file, refactored factory from persisted to federated directives with compose/link processing, added link extraction utilities and FEDERATED_DIRECTIVE_DATAS, introduced new result types for link/schema extraction, and updated type keys to use typed aliases.
Validation Infrastructure
composition/src/validation/types/params.ts, composition/src/validation/validation.ts
New validation module with parameter types and three validators: isArgumentValueValid (literal coercion), validateCustomDirective (argument validation with errors), validateDirectives (directive location/repeatability/default-value validation).
V1 Utilities and Warnings
composition/src/v1/schema-building/type-merging.ts, composition/src/v1/utils/utils.ts, composition/src/v1/warnings/params.ts, composition/src/v1/warnings/warnings.ts
Updated type import paths for deeper modules, removed getNodeCoords export, added InvalidRepeatedComposedDirectiveWarningParams and two new warning constructors (composedOneOfDirectiveWarning, invalidRepeatedComposedDirectiveWarning).
Test Updates
composition/tests/v1/directives/compose-directive.test.ts, composition/tests/v1/directives/override.test.ts, composition/tests/v1/events.test.ts, composition/tests/v1/federation-factory.test.ts, composition/tests/v1/router-configuration.test.ts
New comprehensive test suite for @composeDirective covering normalization and federation; updated tests to use BatchNormalizer class, changed internal mapping references from internalSubgraphBySubgraphName to internalSubgraphByName, and updated error assertions to use new structured error constructors.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'fix: (test) changes to v20 support' does not accurately describe the main changes in this pull request, which involve substantial implementation of @composeDirective functionality, directive definition data structures, refactoring from persisted to federated directives, and related schema-building/federation changes. Update the title to reflect the primary change, such as 'feat: implement @composeDirective with federated directive data structures' or 'refactor: migrate from persisted to federated directives with @composeDirective support'.
Docstring Coverage ⚠️ Warning Docstring coverage is 2.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

❌ Patch coverage is 83.88869% with 440 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.28%. Comparing base (403bd79) to head (0036882).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
composition/src/v1/normalization/utils.ts 47.98% 142 Missing ⚠️
composition/src/errors/errors.ts 35.71% 108 Missing ⚠️
composition/src/validation/validation.ts 78.82% 47 Missing ⚠️
...tion/src/v1/normalization/normalization-factory.ts 82.35% 39 Missing ⚠️
...omposition/src/v1/federation/federation-factory.ts 90.14% 33 Missing ⚠️
composition/src/schema-building/utils.ts 90.74% 25 Missing ⚠️
composition/src/v1/warnings/warnings.ts 8.00% 23 Missing ⚠️
...malization/batch-normalization/batch-normalizer.ts 94.90% 13 Missing ⚠️
composition/src/schema-building/ast.ts 68.75% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2806      +/-   ##
==========================================
- Coverage   46.28%   40.28%   -6.00%     
==========================================
  Files        1045      762     -283     
  Lines      139782   100686   -39096     
  Branches     8768     5142    -3626     
==========================================
- Hits        64702    40565   -24137     
+ Misses      73328    58421   -14907     
+ Partials     1752     1700      -52     
Files with missing lines Coverage Δ
...ctive-definition-data/directive-definition-data.ts 100.00% <100.00%> (ø)
composition/src/directive-definition-data/utils.ts 100.00% <100.00%> (ø)
composition/src/federation/federation.ts 100.00% <100.00%> (ø)
composition/src/federation/types/results.ts 100.00% <ø> (ø)
composition/src/index.ts 100.00% <100.00%> (ø)
composition/src/normalization/normalization.ts 100.00% <ø> (ø)
composition/src/schema-building/types/types.ts 100.00% <ø> (ø)
composition/src/subgraph/types/params.ts 100.00% <100.00%> (ø)
composition/src/subgraph/utils.ts 100.00% <100.00%> (ø)
composition/src/types/results.ts 100.00% <100.00%> (ø)
... and 26 more

... and 342 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 29, 2026

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-cb21d7c3e5bee4264299cad2db1788d88a85ad22-nonroot

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
composition/src/normalization/normalization.ts (1)

1-34: ⚠️ Potential issue | 🟠 Major

Keep a compatibility path for batchNormalize.

Dropping batchNormalize from this entrypoint also removes it from the package root, because composition/src/index.ts re-exports ./normalization/normalization. Any existing import { batchNormalize } ... consumer now breaks unless this is staged as an explicit breaking change or kept behind a deprecated shim.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/normalization/normalization.ts` around lines 1 - 34, The
package removed the batchNormalize export from this normalization entrypoint
causing downstream imports (import { batchNormalize } ...) to break; restore a
compatibility export by reintroducing batchNormalize (or a deprecated shim that
calls the new implementation) alongside normalizeSubgraph and
normalizeSubgraphFromString so composition's re-export still exposes
batchNormalize; locate where normalizeSubgraph/normalizeSubgraphFromString are
forwarded (functions normalizeSubgraph, normalizeSubgraphFromString and the V1
implementations normalizeSubgraphV1/normalizeSubgraphFromStringV1) and add an
exported function named batchNormalize that delegates to the appropriate V1
batchNormalize implementation (or wraps it and emits a deprecation warning) to
preserve backwards compatibility.
composition/src/v1/normalization/utils.ts (1)

467-512: ⚠️ Potential issue | 🔴 Critical

Don't reuse mutable DirectiveDefinitionData singletons here.

initializeDirectiveDefinitionDatas() and FEDERATED_DIRECTIVE_DATAS both hand out the imported *_DEFINITION_DATA objects by reference. Those instances are later mutated in composition/src/v1/normalization/normalization-factory.ts Lines 767-768 and in this file on Lines 531-536, so one normalization run can leak version, isComposed, or description into the next. Please clone or rehydrate fresh DirectiveDefinitionData instances before storing them in these collections.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/v1/normalization/utils.ts` around lines 467 - 512, The
collection initializers are storing references to mutable singleton
DirectiveDefinitionData objects, causing mutations (e.g., version, isComposed,
description) in normalization runs to leak across runs; update
initializeDirectiveDefinitionDatas() and FEDERATED_DIRECTIVE_DATAS to store
fresh copies instead of the imported *_DEFINITION_DATA singletons — for each
symbol like AUTHENTICATED_DEFINITION_DATA, COMPOSE_DIRECTIVE_DEFINITION_DATA,
etc., create a new DirectiveDefinitionData instance or shallow-clone the object
(so fields such as version/isComposed/description are not shared) before
inserting into the Map or the FEDERATED_DIRECTIVE_DATAS array; ensure the same
cloning approach is used wherever those collections are built so
normalization-factory.ts mutations no longer affect subsequent runs.
🧹 Nitpick comments (12)
composition/src/utils/params.ts (1)

16-19: Use an interface for this object-shape params contract.

MergeSetValueMapParams is modeling an object shape and should be an interface per repo TypeScript guidelines.

♻️ Proposed change
-export type MergeSetValueMapParams<K, V> = {
+export interface MergeSetValueMapParams<K, V> {
   source: Map<K, Set<V>>;
   target: Map<K, Set<V>>;
-};
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/utils/params.ts` around lines 16 - 19, Replace the type alias
with an interface for the object-shaped params: change the declaration of
MergeSetValueMapParams to an interface that declares source: Map<K, Set<V>> and
target: Map<K, Set<V>> so it follows the repo TypeScript guideline to use
interfaces for object-shaped contracts; keep the generic parameters <K, V> and
the property names exactly as in MergeSetValueMapParams to preserve usage
compatibility.
composition/src/errors/types/types.ts (1)

3-6: Use an interface instead of a type alias for this object shape.

Line 3 defines an object shape with type; switch to interface to match repo standards.

♻️ Proposed change
-export type InvalidRootTypeFieldEventsDirectiveData = {
+export interface InvalidRootTypeFieldEventsDirectiveData {
   definesDirectives: boolean;
   invalidDirectiveNames: Array<DirectiveName>;
-};
+}

As per coding guidelines, **/*.{ts,tsx}: Prefer interfaces over type aliases for object shapes in TypeScript.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/errors/types/types.ts` around lines 3 - 6, Replace the
exported type alias InvalidRootTypeFieldEventsDirectiveData with an exported
interface of the same name (preserving the property names and types:
definesDirectives: boolean and invalidDirectiveNames: Array<DirectiveName>) so
the object shape uses an interface instead of a type; keep the export and the
DirectiveName reference unchanged.
composition/src/subgraph/types/params.ts (1)

4-7: Prefer an interface for this params object shape.

Line 4 currently uses a type alias for an object shape; this repo standard prefers an interface.

♻️ Proposed change
-export type InternalSubgraphFromNormalizationParams = {
+export interface InternalSubgraphFromNormalizationParams {
   normalization: NormalizationSuccess;
   subgraphName: SubgraphName;
-};
+}

As per coding guidelines, **/*.{ts,tsx}: Prefer interfaces over type aliases for object shapes in TypeScript.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/subgraph/types/params.ts` around lines 4 - 7, Replace the
exported type alias InternalSubgraphFromNormalizationParams with an exported
interface of the same name describing the same properties (normalization:
NormalizationSuccess; subgraphName: SubgraphName) so the object shape uses an
interface instead of a type alias; keep the same exported identifier and
referenced types (NormalizationSuccess, SubgraphName) so all callers remain
compatible.
composition/src/schema-building/types/params.ts (1)

13-75: Prefer interfaces for object-shape params in this new TS types module.

These declarations are all object-shape aliases; convert to interface for consistency with repo TS guidelines.

♻️ Refactor pattern
-export type IsTypeValidImplementationParams = {
+export interface IsTypeValidImplementationParams {
   concreteTypeNamesByAbstractTypeName: Map<TypeName, Set<TypeName>>;
   implementationType: TypeNode;
   interfaceImplementationTypeNamesByInterfaceTypeName: Map<InterfaceTypeName, Set<InterfaceTypeName>>;
   originalType: TypeNode;
-};
+}

As per coding guidelines **/*.{ts,tsx}: Prefer interfaces over type aliases for object shapes in TypeScript.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/schema-building/types/params.ts` around lines 13 - 75,
Several exported object-shaped type aliases (e.g.,
IsTypeValidImplementationParams, GetRouterPersistedDirectiveNodesParams,
GetValidArgumentNodesParams, GetValidExecutableDirectiveArgumentNodesParams,
DirectiveDefinitionNodeFromDataParams, SanitizeDefaultValueParams,
RouterSchemaFieldNodeFromDataParams, RouterSchemaInputValueNodeFromDataParams,
RouterSchemaNodeFromDataParams, CompareAndValidateInputDefaultValuesParams)
should be converted to interfaces; change each "export type X = { ... }" to
"export interface X { ... }", preserving all property names and types (including
Maps, unions, optional fields like node? and description?), keep the same
exported identifiers so external usage/imports remain valid, and run TypeScript
type-check to confirm no downstream changes are required.
composition/src/validation/types/params.ts (1)

6-26: Use interfaces for these exported parameter objects.

All three additions are parameter-bag object shapes, so exporting them as interfaces would align this module with the repo’s TypeScript conventions.

As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/validation/types/params.ts` around lines 6 - 26, Replace the
three exported object-shape type aliases IsArgumentValueValidParams,
ValidateCustomDirectiveParams, and ValidateDirectivesParams with exported
interfaces of the same names and identical member signatures (argumentValue,
parentDefinitionDataByTypeName, typeNode for IsArgumentValueValidParams;
argumentDataByName, directiveNode, parentDefinitionDataByTypeName,
requiredArgumentNames for ValidateCustomDirectiveParams; data, directiveCoords,
directiveDefinitionData, directiveNodes, parentDefinitionDataByTypeName for
ValidateDirectivesParams) so the module follows the repo convention of using
interfaces for parameter-bag objects; keep all property types and export names
unchanged.
composition/src/schema-building/types/types.ts (1)

181-186: Prefer an interface for FederatedDirectivesData.

This is a plain exported object contract, so making it an interface would keep it aligned with the repo’s TypeScript style.

As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/schema-building/types/types.ts` around lines 181 - 186,
Replace the exported type alias FederatedDirectivesData with an exported
interface of the same name and identical members (deprecatedReason: string;
directivesByName: Map<DirectiveName, ConstDirectiveNode[]>; isDeprecated:
boolean; tagDirectiveByName: Map<string, ConstDirectiveNode>), so callers
continue to work without behavior changes; update any nearby JSDoc/comments if
needed and ensure the exported symbol name remains FederatedDirectivesData.
composition/src/v1/normalization/batch-normalization/types/params.ts (1)

3-7: Prefer an interface for this exported params bag.

HandleOverridesParams is a plain object contract, so using an interface here would match the repo’s TS style and keep future extension simpler.

As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/v1/normalization/batch-normalization/types/params.ts` around
lines 3 - 7, Replace the exported type alias HandleOverridesParams with an
exported interface of the same name and identical properties
(originalTypeNameByRenamedTypeName,
overriddenFieldNamesByParentTypeNameByTargetSubgraphName, subgraphName) so the
object contract is defined as an interface (preserve the Map/Set types and their
names) to match the repo style and enable easier future extension; update any
local references if needed but keep the exported symbol name unchanged.
composition/src/v1/federation/federation-factory.ts (1)

1507-1549: Drop the dead persisted-directive stub.

Keeping the old implementation fully commented-out here makes the new directive merge path harder to follow and leaves removed terminology in the hot path. A short TODO or issue link is easier to maintain than carrying the retired code inline.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/v1/federation/federation-factory.ts` around lines 1507 -
1549, Remove the large commented-out implementation inside
upsertPersistedDirectiveDefinitionData(): delete the dead code block (the
commented logic referencing potentialPersistedDirectiveDefinitionDataByName,
setMutualExecutableLocations, upsertInputValueData, setLongestDescription,
addIterableToSet, etc.) and either remove the empty function if it's unused or
replace it with a single-line TODO/issue-link comment explaining why
persisted-directive logic was retired; ensure no removed symbols are referenced
elsewhere or update callsites accordingly.
composition/src/index.ts (1)

3-5: Remove the duplicate barrel re-exports.

./directive-definition-data/directive-definition-data and ./v1/constants/constants are both exported twice. It is harmless, but it makes the public surface harder to audit and easier to create merge conflicts around.

Also applies to: 41-52

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/index.ts` around lines 3 - 5, The file contains duplicate
barrel re-exports (e.g., the export of
'./directive-definition-data/directive-definition-data' and
'./v1/constants/constants' appear more than once); remove the duplicate export
statements so each module is exported only once—locate the repeated export lines
for directive-definition-data and v1/constants in index.ts (also the repeated
block around lines 41-52) and delete the redundant entries, leaving a single
canonical export for each module.
composition/src/directive-definition-data/types/params.ts (1)

21-67: Use interfaces for these exported param objects.

These new declarations are all object shapes, and the repo guideline prefers interface here over type = { ... }.

As per coding guidelines "Prefer interfaces over type aliases for object shapes in TypeScript".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/directive-definition-data/types/params.ts` around lines 21 -
67, Change the exported object-shaped type aliases to exported interfaces:
replace the type declarations for UpsertFederatedDirectiveDataParams,
AddDirectiveArgumentDataByNodeParams, DirectiveArgumentDataParams,
DirectiveDefinitionDataParams, and ExtractDirectiveArgumentDataParams with
equivalent exported interfaces, preserving all property names, optional
modifiers (e.g. ?), exact property types (Maps, Sets, unions like InputNodeKind
| Kind.NULL), comments (e.g. the commented configureDescription line), and
exported status so consumers and imports remain unchanged.
composition/src/schema-building/utils.ts (1)

211-216: Use strict equality for the printed default-value comparison.

This is a string-to-string comparison, so === keeps it consistent with the repo rule and avoids introducing a loose-equality exception into the new path.

As per coding guidelines "Always use strict equality (=== and !==) instead of loose equality (== and !=)".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/schema-building/utils.ts` around lines 211 - 216, Replace the
loose equality check between printed default-value strings with a strict
equality check: in the block that computes existingDefaultValueString =
print(existingData.defaultValue) and incomingDefaultValueString =
print(incomingData.defaultValue), change the comparison from `==` to `===` so
the early-return path in the function uses strict equality
(`existingDefaultValueString === incomingDefaultValueString`) consistent with
project guidelines.
composition/src/normalization/types.ts (1)

36-63: Prefer interfaces for these result payloads.

NormalizationSuccess and BatchNormalizationSuccess are plain object contracts, so keeping them as interfaces would align better with the repo TS convention and make later extension/composition easier.

As per coding guidelines "Prefer interfaces over type aliases for object shapes in TypeScript".

Also applies to: 72-84

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@composition/src/normalization/types.ts` around lines 36 - 63, Replace the
object type aliases with interfaces for the normalization result payloads:
change the NormalizationSuccess type declaration to an interface named
NormalizationSuccess and do the same for BatchNormalizationSuccess (the other
alias referenced in the comment around lines 72-84), keeping all property names
and types identical; ensure exported declarations remain exported and update any
import/usage sites if they rely on type-only constructs so the interface names
are used consistently across functions like any normalizer return types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/actions/node/action.yaml:
- Around line 5-7: The description for the node-version input is inconsistent
with the actual default; update the "description" string for the node-version
input so it matches the configured default value ('22.x') or change the
"default" to 'lts' if you intend to keep the original message—modify the
description line that currently reads "The node version to install (Default:
lts)" to reflect "Default: 22.x" (or set default to 'lts') so the node-version
input's description and the default: '22.x' stay aligned.

In `@composition/package.json`:
- Line 43: Update the composition package catalog entry for "@types/node" so it
permits Node 24 typings as well as 22; specifically replace the current catalog
pin ("@types/node": "catalog:") value with a version range that covers both
major versions (for example "^22.0.0 || ^24.0.0" or another appropriate
inclusive range) so TypeScript will resolve `@types/node` matching Node 24 when
running on Node 24, and ensure the change is made to the "@types/node"
dependency entry in composition/package.json.

In `@composition/src/schema-building/utils.ts`:
- Around line 600-602: When building schema nodes, ensure you clear
data.node.defaultValue when data.includeDefaultValue is false so previous
defaults don't leak; in the code handling data.includeDefaultValue (refer to the
data.includeDefaultValue check and the mutated data.node.defaultValue
assignment), add logic to remove or set data.node.defaultValue to undefined/null
in the else path (or before returning) so that when includeDefaultValue is false
any previously-set default is explicitly cleared.
- Around line 472-484: The current early-return path skips validation for single
directive instances; remove or change the short-circuit so that you always run
validateDirectives on the (possibly single) directive node(s). Specifically,
stop pushing directiveNodes directly and instead call
extractUniqueDirectiveNodes(directiveNodes) and then validateDirectives({ data,
directiveCoords: coords, directiveDefinitionData: directiveData, directiveNodes:
uniqueDirectiveNodes, parentDefinitionDataByTypeName }) for all cases (including
when length === 1); only push nodes into nodes after validation passes.

In `@composition/src/subgraph/utils.ts`:
- Line 21: The constructor/creator for InternalSubgraph is currently resetting
overriddenFieldNamesByParentTypeName to a new empty Map, which drops normalized
override metadata; instead copy or clone the existing override map from the
source subgraph into the new InternalSubgraph so override-aware logic keeps
working. Locate where InternalSubgraph is built (the place that sets
overriddenFieldNamesByParentTypeName) and replace the new Map() assignment with
a shallow copy/clone of the original map (preserving each Set<FieldName> per
TypeName) so callers that rely on overriddenFieldNamesByParentTypeName continue
to see the normalized override data.

In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 733-745: handleComposeDirective currently assumes the first
argument is a string and calls startsWith on rawValue; add a guard in
handleComposeDirective (operating on the ComposeDirectiveNode and its argNode)
to verify the first argument is a string literal before using string
methods—e.g., check argNode.value.kind === 'StringValue' or typeof
argNode.value.value === 'string' and if that check fails push a composition
error (or reuse the existing directive error collection) and return early so
malformed non-string values (objects/booleans) do not cause a runtime exception
when evaluating rawValue.startsWith(LITERAL_AT).

In `@composition/src/v1/normalization/types/types.ts`:
- Around line 9-31: The ComposeDirectiveNode and ComposeDirectiveArgumentNode
types reference loc?: Location but Location isn't imported from 'graphql';
update the existing graphql import (the block that currently imports
ConstDirectiveNode, DocumentNode, InputValueDefinitionNode, Kind, NameNode,
StringValueNode, ValueNode) to also import the type Location so the loc property
resolves correctly for those types.

In `@composition/src/v1/warnings/warnings.ts`:
- Around line 211-238: The two warning message strings need copy fixes: in
composedOneOfDirectiveWarning update the concatenated message so "federated.
schema" becomes "federated schema" (remove the stray period and join sentences
cleanly) and in invalidRepeatedComposedDirectiveWarning remove the double space
in "defined  as" (make it "defined as") and tidy any surrounding
spacing/concatenation so the final Warning message reads smoothly; locate these
messages in the functions composedOneOfDirectiveWarning and
invalidRepeatedComposedDirectiveWarning and adjust the template strings
accordingly.

In `@composition/src/validation/validation.ts`:
- Around line 118-122: The current early return only checks that the named type
is an INPUT_OBJECT_TYPE_DEFINITION and that the argumentValue is an OBJECT, but
does not validate fields; replace the TODO by performing a deep comparison: when
parentData.kind === Kind.INPUT_OBJECT_TYPE_DEFINITION and argumentValue.kind ===
Kind.OBJECT, iterate the input object type definition (parentData) fields and
verify required fields are present, each provided field matches the field's
input type (recursively validate nested OBJECT/LIST/SCALAR values), and reject
unexpected fields not defined on parentData; reuse or call the existing
value-validation helper (or implement a recursive validateValueAgainstInputType
function) to validate nested values and ensure non-nullable constraints and
default behavior are enforced for symbols parentData, argumentValue, and Kind.
- Around line 108-110: The validator currently allows quoted strings for
enum-typed directive arguments by checking argumentValue.kind against
Kind.STRING; update the check in the block where parentData.kind ===
Kind.ENUM_TYPE_DEFINITION to reject Kind.STRING and only accept Kind.ENUM (i.e.,
remove or change the branch that treats Kind.STRING as valid) so
argumentValue.kind must equal Kind.ENUM to return true and everything else
returns false; adjust the logic around parentData.kind,
Kind.ENUM_TYPE_DEFINITION, and argumentValue.kind accordingly.

In `@composition/tests/v1/directives/compose-directive.test.ts`:
- Line 250: The placeholder test "that different core features can import the
same named directive if only one is composed" is empty and gives false coverage;
either implement real assertions that exercise the scenario or remove the test
until covered. Implementing: create two mock core feature modules that both
import a directive with the same name, compose only one of them via the
composeDirective/composeFeature helper used elsewhere in this suite, then assert
that importing the uncomposed feature does not produce a composed directive (or
produces a fallback) while the composed feature yields the composed directive;
use the existing test helpers and functions in compose-directive.test.ts (the
test block with that description) to locate where to add these assertions.
Ensure assertions check both feature-level import behavior and that only one
composed directive instance exists.

---

Outside diff comments:
In `@composition/src/normalization/normalization.ts`:
- Around line 1-34: The package removed the batchNormalize export from this
normalization entrypoint causing downstream imports (import { batchNormalize }
...) to break; restore a compatibility export by reintroducing batchNormalize
(or a deprecated shim that calls the new implementation) alongside
normalizeSubgraph and normalizeSubgraphFromString so composition's re-export
still exposes batchNormalize; locate where
normalizeSubgraph/normalizeSubgraphFromString are forwarded (functions
normalizeSubgraph, normalizeSubgraphFromString and the V1 implementations
normalizeSubgraphV1/normalizeSubgraphFromStringV1) and add an exported function
named batchNormalize that delegates to the appropriate V1 batchNormalize
implementation (or wraps it and emits a deprecation warning) to preserve
backwards compatibility.

In `@composition/src/v1/normalization/utils.ts`:
- Around line 467-512: The collection initializers are storing references to
mutable singleton DirectiveDefinitionData objects, causing mutations (e.g.,
version, isComposed, description) in normalization runs to leak across runs;
update initializeDirectiveDefinitionDatas() and FEDERATED_DIRECTIVE_DATAS to
store fresh copies instead of the imported *_DEFINITION_DATA singletons — for
each symbol like AUTHENTICATED_DEFINITION_DATA,
COMPOSE_DIRECTIVE_DEFINITION_DATA, etc., create a new DirectiveDefinitionData
instance or shallow-clone the object (so fields such as
version/isComposed/description are not shared) before inserting into the Map or
the FEDERATED_DIRECTIVE_DATAS array; ensure the same cloning approach is used
wherever those collections are built so normalization-factory.ts mutations no
longer affect subsequent runs.

---

Nitpick comments:
In `@composition/src/directive-definition-data/types/params.ts`:
- Around line 21-67: Change the exported object-shaped type aliases to exported
interfaces: replace the type declarations for
UpsertFederatedDirectiveDataParams, AddDirectiveArgumentDataByNodeParams,
DirectiveArgumentDataParams, DirectiveDefinitionDataParams, and
ExtractDirectiveArgumentDataParams with equivalent exported interfaces,
preserving all property names, optional modifiers (e.g. ?), exact property types
(Maps, Sets, unions like InputNodeKind | Kind.NULL), comments (e.g. the
commented configureDescription line), and exported status so consumers and
imports remain unchanged.

In `@composition/src/errors/types/types.ts`:
- Around line 3-6: Replace the exported type alias
InvalidRootTypeFieldEventsDirectiveData with an exported interface of the same
name (preserving the property names and types: definesDirectives: boolean and
invalidDirectiveNames: Array<DirectiveName>) so the object shape uses an
interface instead of a type; keep the export and the DirectiveName reference
unchanged.

In `@composition/src/index.ts`:
- Around line 3-5: The file contains duplicate barrel re-exports (e.g., the
export of './directive-definition-data/directive-definition-data' and
'./v1/constants/constants' appear more than once); remove the duplicate export
statements so each module is exported only once—locate the repeated export lines
for directive-definition-data and v1/constants in index.ts (also the repeated
block around lines 41-52) and delete the redundant entries, leaving a single
canonical export for each module.

In `@composition/src/normalization/types.ts`:
- Around line 36-63: Replace the object type aliases with interfaces for the
normalization result payloads: change the NormalizationSuccess type declaration
to an interface named NormalizationSuccess and do the same for
BatchNormalizationSuccess (the other alias referenced in the comment around
lines 72-84), keeping all property names and types identical; ensure exported
declarations remain exported and update any import/usage sites if they rely on
type-only constructs so the interface names are used consistently across
functions like any normalizer return types.

In `@composition/src/schema-building/types/params.ts`:
- Around line 13-75: Several exported object-shaped type aliases (e.g.,
IsTypeValidImplementationParams, GetRouterPersistedDirectiveNodesParams,
GetValidArgumentNodesParams, GetValidExecutableDirectiveArgumentNodesParams,
DirectiveDefinitionNodeFromDataParams, SanitizeDefaultValueParams,
RouterSchemaFieldNodeFromDataParams, RouterSchemaInputValueNodeFromDataParams,
RouterSchemaNodeFromDataParams, CompareAndValidateInputDefaultValuesParams)
should be converted to interfaces; change each "export type X = { ... }" to
"export interface X { ... }", preserving all property names and types (including
Maps, unions, optional fields like node? and description?), keep the same
exported identifiers so external usage/imports remain valid, and run TypeScript
type-check to confirm no downstream changes are required.

In `@composition/src/schema-building/types/types.ts`:
- Around line 181-186: Replace the exported type alias FederatedDirectivesData
with an exported interface of the same name and identical members
(deprecatedReason: string; directivesByName: Map<DirectiveName,
ConstDirectiveNode[]>; isDeprecated: boolean; tagDirectiveByName: Map<string,
ConstDirectiveNode>), so callers continue to work without behavior changes;
update any nearby JSDoc/comments if needed and ensure the exported symbol name
remains FederatedDirectivesData.

In `@composition/src/schema-building/utils.ts`:
- Around line 211-216: Replace the loose equality check between printed
default-value strings with a strict equality check: in the block that computes
existingDefaultValueString = print(existingData.defaultValue) and
incomingDefaultValueString = print(incomingData.defaultValue), change the
comparison from `==` to `===` so the early-return path in the function uses
strict equality (`existingDefaultValueString === incomingDefaultValueString`)
consistent with project guidelines.

In `@composition/src/subgraph/types/params.ts`:
- Around line 4-7: Replace the exported type alias
InternalSubgraphFromNormalizationParams with an exported interface of the same
name describing the same properties (normalization: NormalizationSuccess;
subgraphName: SubgraphName) so the object shape uses an interface instead of a
type alias; keep the same exported identifier and referenced types
(NormalizationSuccess, SubgraphName) so all callers remain compatible.

In `@composition/src/utils/params.ts`:
- Around line 16-19: Replace the type alias with an interface for the
object-shaped params: change the declaration of MergeSetValueMapParams to an
interface that declares source: Map<K, Set<V>> and target: Map<K, Set<V>> so it
follows the repo TypeScript guideline to use interfaces for object-shaped
contracts; keep the generic parameters <K, V> and the property names exactly as
in MergeSetValueMapParams to preserve usage compatibility.

In `@composition/src/v1/federation/federation-factory.ts`:
- Around line 1507-1549: Remove the large commented-out implementation inside
upsertPersistedDirectiveDefinitionData(): delete the dead code block (the
commented logic referencing potentialPersistedDirectiveDefinitionDataByName,
setMutualExecutableLocations, upsertInputValueData, setLongestDescription,
addIterableToSet, etc.) and either remove the empty function if it's unused or
replace it with a single-line TODO/issue-link comment explaining why
persisted-directive logic was retired; ensure no removed symbols are referenced
elsewhere or update callsites accordingly.

In `@composition/src/v1/normalization/batch-normalization/types/params.ts`:
- Around line 3-7: Replace the exported type alias HandleOverridesParams with an
exported interface of the same name and identical properties
(originalTypeNameByRenamedTypeName,
overriddenFieldNamesByParentTypeNameByTargetSubgraphName, subgraphName) so the
object contract is defined as an interface (preserve the Map/Set types and their
names) to match the repo style and enable easier future extension; update any
local references if needed but keep the exported symbol name unchanged.

In `@composition/src/validation/types/params.ts`:
- Around line 6-26: Replace the three exported object-shape type aliases
IsArgumentValueValidParams, ValidateCustomDirectiveParams, and
ValidateDirectivesParams with exported interfaces of the same names and
identical member signatures (argumentValue, parentDefinitionDataByTypeName,
typeNode for IsArgumentValueValidParams; argumentDataByName, directiveNode,
parentDefinitionDataByTypeName, requiredArgumentNames for
ValidateCustomDirectiveParams; data, directiveCoords, directiveDefinitionData,
directiveNodes, parentDefinitionDataByTypeName for ValidateDirectivesParams) so
the module follows the repo convention of using interfaces for parameter-bag
objects; keep all property types and export names unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 86097bfb-346d-4a7f-9e62-613019c5b147

📥 Commits

Reviewing files that changed from the base of the PR and between 403bd79 and 19e7d53.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (61)
  • .github/actions/node/action.yaml
  • .github/workflows/cli-ci.yaml
  • composition-go/index.global.js
  • composition/package.json
  • composition/src/directive-definition-data/directive-definition-data.ts
  • composition/src/directive-definition-data/types/params.ts
  • composition/src/directive-definition-data/types/types.ts
  • composition/src/directive-definition-data/utils.ts
  • composition/src/errors/errors.ts
  • composition/src/errors/types/params.ts
  • composition/src/errors/types/types.ts
  • composition/src/federation/federation.ts
  • composition/src/federation/types/params.ts
  • composition/src/federation/types/results.ts
  • composition/src/federation/types/types.ts
  • composition/src/index.ts
  • composition/src/normalization/normalization.ts
  • composition/src/normalization/types.ts
  • composition/src/schema-building/ast.ts
  • composition/src/schema-building/params.ts
  • composition/src/schema-building/types/params.ts
  • composition/src/schema-building/types/results.ts
  • composition/src/schema-building/types/types.ts
  • composition/src/schema-building/utils.ts
  • composition/src/subgraph/types.ts
  • composition/src/subgraph/types/params.ts
  • composition/src/subgraph/utils.ts
  • composition/src/types/results.ts
  • composition/src/types/types.ts
  • composition/src/utils/params.ts
  • composition/src/utils/string-constants.ts
  • composition/src/utils/utils.ts
  • composition/src/v1/constants/strings.ts
  • composition/src/v1/federation/federation-factory.ts
  • composition/src/v1/federation/types/params.ts
  • composition/src/v1/federation/utils.ts
  • composition/src/v1/federation/walkers.ts
  • composition/src/v1/normalization/batch-normalization/batch-normalizer.ts
  • composition/src/v1/normalization/batch-normalization/types/params.ts
  • composition/src/v1/normalization/batch-normalization/types/results.ts
  • composition/src/v1/normalization/batch-normalization/types/types.ts
  • composition/src/v1/normalization/directive-definition-data.ts
  • composition/src/v1/normalization/normalization-factory.ts
  • composition/src/v1/normalization/types/params.ts
  • composition/src/v1/normalization/types/results.ts
  • composition/src/v1/normalization/types/types.ts
  • composition/src/v1/normalization/utils.ts
  • composition/src/v1/normalization/walkers.ts
  • composition/src/v1/schema-building/type-merging.ts
  • composition/src/v1/utils/utils.ts
  • composition/src/v1/warnings/params.ts
  • composition/src/v1/warnings/warnings.ts
  • composition/src/validation/types/params.ts
  • composition/src/validation/types/results.ts
  • composition/src/validation/validation.ts
  • composition/tests/v1/directives/compose-directive.test.ts
  • composition/tests/v1/directives/override.test.ts
  • composition/tests/v1/events.test.ts
  • composition/tests/v1/federation-factory.test.ts
  • composition/tests/v1/router-configuration.test.ts
  • pnpm-workspace.yaml
💤 Files with no reviewable changes (2)
  • composition/src/schema-building/params.ts
  • composition/src/v1/normalization/directive-definition-data.ts

Comment thread .github/actions/node/action.yaml
Comment thread composition/package.json
Comment thread composition/src/schema-building/utils.ts
Comment thread composition/src/schema-building/utils.ts
Comment thread composition/src/subgraph/utils.ts
Comment thread composition/src/v1/normalization/types/types.ts
Comment thread composition/src/v1/warnings/warnings.ts
Comment thread composition/src/validation/validation.ts
Comment thread composition/src/validation/validation.ts
Comment thread composition/tests/v1/directives/compose-directive.test.ts
@comatory
Copy link
Copy Markdown
Contributor Author

@CodeRabbit stop

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

@comatory Sure, I'll pause the automated reviews for this PR.


🧠 Learnings used
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2689
File: router-tests/operations/testdata/introspection/base-graph-schema.json:2425-2440
Timestamp: 2026-03-25T12:09:05.251Z
Learning: Repo wundergraph/cosmo: In PR `#2689`, WorkSetup.priority is defined as String! in demo/pkg/subgraphs/employees/subgraph/schema.graphqls and reflected as String! in both router-tests/operations/testdata/introspection/base-graph-schema.json and feature-graph-schema.json; a previous automated comment incorrectly claimed an Int! mismatch and referenced WorkSetup on a diff hunk that actually concerned EmployeeWorkItem. For future reviews, verify that flagged mismatches correspond to the specific diff hunk and confirm types directly from SDL and introspection before raising a type-mismatch finding.
✅ Actions performed

Reviews paused.

@github-actions

This comment has been minimized.

@comatory comatory force-pushed the ondrej/eng-3812-implement-composedirective-directive branch from 0e2f3d2 to d248ab0 Compare April 29, 2026 06:39
@comatory comatory force-pushed the ondrej/eng-3812-implement-composedirective-directive branch from d248ab0 to 309328f Compare April 29, 2026 06:42
@comatory comatory closed this Apr 30, 2026
@comatory comatory deleted the ondrej/eng-3812-implement-composedirective-directive branch April 30, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants